Harden log4net against the findings of a security audit - #310

Events are removed from the CyclicBuffer by PopAll before SendBuffer runs,
so they cannot be retried later. Neither ExecuteNonQuery loop contained
per-event failures, and with UseTransactions (the default) a single event
the provider rejects - npgsql refuses U+0000, for example - rolled back
the whole batch of up to 512 events, including the ones logged before it.
An attacker who gets one such byte logged per flush window could suppress
the database audit trail indefinitely.

Without a transaction each event is now reported and skipped individually.
In transaction mode the exception still has to propagate, so the events
are retried one by one after the rollback; only the events the database
actually rejects are lost.

This makes delivery at-least-once: if the batch failed after the database
had already applied some statements, those events are written again.
Duplicates are preferred over losing the whole buffer.

Log4NetTransaction.Dispose threw NotImplementedException, which log4net
swallowed in DoAppend, so no test ever exercised the rollback path. Real
providers roll back on Dispose rather than throwing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Without CommandText the appender builds a complete SQL statement per event
by rendering the Layout and executes it as it is. Layouts perform no SQL
quoting or escaping and offer no way to add it, so anything that reaches a
log statement is executed as part of the statement, with the privileges of
the appender's connection.

ActivateOptions now logs an error naming the appender and pointing at
CommandText with AdoNetAppenderParameter bindings, which pass content as
database parameters. The mode itself keeps working, so no existing
configuration breaks.

The manual gained a warning as well, and its claim that BufferSize
defaults to 100 is corrected to 512.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Clients are written to synchronously while the appender lock is held and no
Socket.SendTimeout was set anywhere, so a client that connects and then
stops reading let TCP flow control fill its receive window and the server
send buffer. The next write blocked forever and every thread logging
through the appender queued behind it. The existing eviction only fires on
a thrown exception, and a blocked write never throws.

Accepted sockets now get a finite SendTimeout, configurable through the new
SendTimeoutMillis property and defaulting to 5000. A timed-out write throws
and the client is evicted like any other dead connection. Setting the
property to 0 restores the previous unbounded behavior.

SocketHandler gained a (port, sendTimeoutMillis) overload rather than an
optional parameter, so the existing (port) signature keeps working for
subclasses; it maps to 0 to preserve its old semantics.

Writes stay synchronous under the appender lock, so several stalled clients
still cost up to the timeout each. Moving the sends to a bounded per-client
queue would remove that entirely and is left as a follow-up.

TelnetAppender had no page in the manual, which is added here, including
that it is a diagnostic tool for trusted networks and that the connecting
client is trusted, like any other appender destination.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

InitializeDatabaseConnection named the resolved connection string in full
when it could not open the connection, and the documented examples embed
Password=... The message goes through the ErrorHandler, so it is what an
operator sees while diagnosing exactly this failure.

Password-bearing keywords are now replaced with *****. The rest of the
connection string is kept, so the message stays useful for spotting a typo
in the server name or catalog. If the string cannot be parsed - likely,
given that it just failed to connect - all of it is redacted.

This matters more since appender errors became visible without
log4net.Internal.Debug: the password would otherwise have reached stderr in
a default configuration.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Security scans regularly report the configuration paths as cleartext
transport, credential exposure, unrestricted type loading and unverified
reload. None of them is a vulnerability, so record why, with a link to the
threat model, at the places a scan actually flags:

- InternalConfigure(ILoggerRepository, Uri) neither restricts the URI scheme
  nor withholds the process credentials. The endpoint is named by the
  configuration and is trusted for the same reason an appender destination
  is; transmitting configuration confidentially is a deployer
  responsibility. Nothing runs until an operator supplies a URI, either by
  calling Configure(Uri) or through the log4net.Config appSetting.
- ParseAppender instantiates the types the configuration names, and
  SetParameter reaches non-public members, both by design.
- ConfigureAndWatchHandler reloads a replaced file without re-checking its
  origin; keeping the watched file writable only by the operator is a
  deployer responsibility.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

EnableSsl mapped to MailKit's SecureSocketOptions.Auto, which is
opportunistic on every port other than 465. An attacker able to strip
STARTTLS from the EHLO response silently downgraded the session to
plaintext, taking the credentials passed to Authenticate and the log
content with it, while the operator had asked for an encrypted connection.

EnableSsl now requires transport security: implicit TLS on port 465 and
mandatory STARTTLS elsewhere, so connecting fails when the server offers no
TLS, as System.Net.Mail.SmtpClient.EnableSsl does. The appender ships for
the first time in this release, so no configuration changes behaviour.

Opportunistic STARTTLS is still reachable, but only by asking for it. The
new TransportSecurity option carries the full set of modes and EnableSsl
became a shorthand for it, so the two cannot disagree. TransportSecurity
also covers a server expecting implicit TLS on a port other than 465, which
neither Auto nor the legacy appender could reach.

The option uses its own enum rather than MailKit's SecureSocketOptions,
which is not CLS compliant and would have needed CLSCompliant(false) on the
primary TLS setting.

The remarks on EnableSsl offered a custom ISmtpTransport for finer control,
which no caller can supply because both the interface and the constructor
taking it are internal. Removed, since TransportSecurity is the answer now.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The rendered message is marshaled to libc as a null-terminated string, so a
NUL character anywhere in it ended the record there and silently dropped
everything the layout rendered after it, including trailing fields and
exception text. Logged content is not trusted and a NUL in it is an
in-scope input, so an attacker who gets one logged could hide the tail of
every record.

Confirmed with the same marshalling the appender uses: for a 24 character
message with a NUL in the middle, libc sees 13 characters.

NUL is now escaped as \0. Other control characters are still passed
through, because syslog(3) encodes them itself and newlines are needed for
the multi-line output an exception layout produces. RemoteSyslogAppender
drops unprintable characters instead, which would lose the stack traces
this appender is expected to carry.

Also switches the single statement tests added for the send timeout to
expression bodies.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The Identity becomes the TAG of the syslog record and was appended
verbatim, two lines before the message part goes through AppendMessage's
filtering. A carriage return or line feed in the TAG ends the record, so the
text after it is read as a record of its own with its own facility and
severity.

The TAG is a structural identifier and is expected to be a constant rather
than a pattern rendering event data, so a malformed one is a configuration
error. It is now reported through the ErrorHandler instead of being repaired
quietly.

The control characters are removed rather than the event being dropped. An
Identity pattern that does render event data would otherwise give control
over whether a record survives at all.

Only control characters are removed. Identity defaults to the application
friendly name, which may contain a space, and a space cannot split the
record.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

RegexToMatch was compiled with Regex.InfiniteMatchTimeout, and the match
runs while the appender lock is held, so a pattern that backtracks could
stall everything logging through the appender on some inputs.

Matching now stops after MatchTimeoutMillis, 1000 by default, and 0 restores
the previous unbounded behaviour. An abandoned match counts as no match, so
the event is left to the rest of the filter chain rather than having its
decision changed.

The pattern comes from configuration and is trusted, so this is hardening
against a pattern that turns out to be expensive, not protection against
untrusted input.

StringMatchFilter and PropertyFilter both matched the regex themselves, so
the handling lives in one protected IsRegexMatch used by both, which also
covers MdcFilter and NdcFilter. It reports an abandoned match once per
filter rather than once per event, since a warning per event would be a
problem of its own.

regexToMatch was missing from the manual entirely and is documented now.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Flush synchronized on a private object while Append runs under the lock
taken by DoAppend, so a flush could run concurrently with a write to the
same QuietTextWriter, which is not thread safe. The comment claiming the
lock blocked any Append was left over from when it locked on this.

All four lock sites in the class now take the inherited LockObj and the
private object is gone, so no ordering between two locks remains. Taking
LockObj in OnClose is safe because Close already holds it and Monitor is
reentrant.

Flush also returned true whatever happened. QuietTextWriter routes failing
writes to the ErrorHandler but does not override Flush, so a failure from
the underlying writer escaped to the caller. It is now reported with
ErrorCode.FlushFailure and Flush returns false.

The AdoNet test doubles gained the doc comments the rest of the test code
has, matching Log4NetTransaction.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

UserName resolved the identity of whichever thread read it, so with a
buffering appender the buffered events were attributed to the thread
flushing the buffer rather than the one that logged them.

Only impersonation makes that wrong. Without it the name is the process
identity, which is the same on every thread and stays resolvable, so it is
still resolved lazily and nothing changes for those applications.

An event logged while impersonating now takes its user name with it when it
is fixed, because that is the last point at which the identity is known.
This happens whatever Fix asks for, and outside the block that fixes the
requested fields, since that block is skipped when there is nothing to fix
while the cache is locked all the same. FixFlags.UserName stays unset, so
the flags keep reporting what the caller requested.

Reading the property on a thread that is impersonating no longer reports
that thread's user; the not available text is used instead.

The impersonation check only queries the thread token. Resolving the name
behind it is the expensive part and is unchanged.

Also renames FixingTest.All_ShouldContainAllFlags and documents its members.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Composite formatting honours the alignment of a format item before anything
can reject it, so a format such as "{0,2000000000}" allocates a buffer of
that size, and the OutOfMemoryException is fatal and escapes the catch that
otherwise turns a bad format into an error string. Security scans report
this, so record why it is not guarded against, with a link to the threat
model.

Format strings are developer-controlled and trusted, and routing user data
into one is application misuse.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

verify-release.ps1 reported a SHA-512 mismatch with -ErrorAction Continue,
which overrides the script level $ErrorActionPreference, and never checked
the exit code of gpg, because $ErrorActionPreference does not apply to
native commands. A tampered artifact or a broken signature therefore still
reached Expand-Archive and the script exited 0. build-release.ps1 already
sets $PSNativeCommandUseErrorActionPreference with a comment explaining
this, so the trap was known.

Both scripts looped over whichever .asc files happened to be present, so
deleting them left nothing to verify and the scripts succeeded. The .sha512
files travel with the artifacts and can be regenerated, so they add nothing
on their own.

Verification is now driven from the artifacts: everything that is not a
hash, a signature or KEYS must have both, and an empty directory is an
error. That also catches a file added to the release.

Both scripts now import KEYS into a key ring of their own. Importing into
the default one accepted a signature from any key the machine already
trusted rather than only from a key in the Logging Services KEYS file.

Checked against a synthetic release signed with a throwaway key. Before,
the PowerShell script accepted a tampered artifact, a corrupted signature,
a deleted signature, an added unsigned file and a signature from an
untrusted key, and the shell script accepted the last three. After, all of
them are rejected and an untampered release still passes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

openlog keeps the pointer it is given rather than a copy of the string, and
registers it for the process rather than for an appender.

ActivateOptions allocated a new buffer and overwrote the handle to the
previous one without freeing it, so every re-activation leaked a buffer. The
handle is now replaced under a lock, and the old buffer is freed only once
openlog points at the new string. A failing openlog frees the new one.

The handle became static, which is what the registration already was: a
second appender replaces the identity of the first rather than adding one.

OnClose no longer frees it. The buffer belongs to a process wide
registration that outlives the appender, and another instance may still be
logging through it. That closelog ends the connection for every instance is
now documented as well.

The use-after-free the audit describes does not apply to the libcs log4net
targets: closelog runs before the free and glibc clears its stored pointer,
while musl copies the ident. Only the leak and the shared lifetime are
fixed here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

InterProcessLock.AcquireLock carried a "TODO: add timeout?" and waited
without one, as did RollingFileAppender when deciding whether to roll. Both
waits happen while the appender lock is held, so a mutex nobody releases
suspended every thread logging through the appender.

Both now wait at most LockTimeoutMillis, 10000 by default, with
Timeout.Infinite restoring the previous behaviour. An event that cannot get
the file lock is reported and dropped; one that cannot get the rolling lock
is written to the current file without the roll check, because rolling
without the lock would race another process renaming the same files.

Two more problems turned up on the way:

AbandonedMutexException was unhandled although it means the wait succeeded
and this thread owns the mutex. It propagated out of AcquireLock before
_recursiveWatch was incremented, so ReleaseLock never released it and the
mutex stayed held for good. That needs no attacker, only a process dying
mid-write.

AdjustFileBeforeAppend released the rolling mutex in a finally without
checking that it had been taken. Harmless while the wait could only succeed,
but a bounded wait makes it throw, so it is guarded now.

The mutex names are left alone. They are derived from the log file path so
that separate processes agree on them, and making them unpredictable would
break the cross-process coordination they exist for.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The listening socket was bound to IPAddress.Any with no way to scope it, so
an operator who only wanted to watch the log from the machine itself still
got a listener on every interface.

ListenAddress fills that gap. The default is unchanged, so nothing moves
unless it is set: the connecting client is trusted, as the manual now
states, and flipping the default would break every remote monitoring setup
on upgrade.

The listening socket now takes its family from the address rather than
always being InterNetwork, so an IPv6 address works too.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

mvnw and MavenWrapperDownloader both refuse to run when a download does not
match a checksum, but neither wrapperSha256Sum nor distributionSha256Sum was
set, so the enforcement never ran and whatever the URLs returned was
executed.

wrapperSha256Sum is of the maven-wrapper.jar committed next to the
properties, which is byte identical to the published maven-wrapper-3.2.0.jar.

distributionSha256Sum is of apache-maven-3.9.0-bin.zip from
archive.apache.org, whose SHA-512 matches the published value, which is byte
identical to the copy on Maven Central, and whose PGP signature verifies
against https://downloads.apache.org/maven/KEYS.

Both enforcement paths were exercised: with the checksums correct mvnw runs
Maven 3.9.0, and with either one altered it refuses.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Its push and pull request triggers had been commented out, leaving it
dispatched by hand only, so nothing depends on it.

What remained was worth removing rather than pinning: it ran
npx git-broadcast@beta in a job that checks out with a token able to push to
this repository, using mutable action tags and with no permissions block.
The beta dist-tag resolves to 0.45.7 from 2024, older than the 0.50.0 that
latest points at, so it was not tracking newer code either; it was simply a
pointer that can be moved to any published version.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>